Feature: Add Gemini and Z.ai (GLM) usage tracking. Improve task recovery and remove unused variables#773
Conversation
📝 WalkthroughWalkthroughAdds z.ai and Gemini integrations across server and UI: new usage services, API routes (/api/zai, /api/gemini), UI hooks, store types/state, settings migration for zai API key, provider configuration UI, and usage display components. Also includes various typing, error-handling, and minor cleanup changes. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI Component
participant Hook as useZaiUsage Hook
participant Client as HttpApiClient
participant Server as Server (/api/zai)
participant Service as ZaiUsageService
participant ZaiAPI as z.ai API
UI->>Hook: mount / request refresh
Hook->>Client: http.zai.getUsage()
Client->>Server: GET /api/zai/usage
Server->>Service: fetchUsageData()
Service->>ZaiAPI: POST /quota (with token)
ZaiAPI-->>Service: Quota response
Service->>Server: ZaiUsageData
Server-->>Client: ZaiUsageData
Client-->>Hook: ZaiUsageData
Hook->>UI: update store -> render usage
sequenceDiagram
participant Hook as useProviderAuthInit
participant Client as HttpApiClient
participant Server as Server (/api/gemini)
participant Service as GeminiUsageService
participant CLI as Gemini CLI
Hook->>Client: http.setup.getGeminiStatus()
Client->>Server: GET /api/gemini/status
Server->>Service: create provider & detectInstallation()
Service->>CLI: locate binary & read oauth2.js
CLI-->>Service: oauth credentials / version
Service-->>Server: status payload (installed/authenticated/method)
Server-->>Client: status
Client-->>Hook: status
Hook->>Store: setGeminiAuthStatus(status)
(Note: rectangles/colors not required for simple sequence flow.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @gsxdsm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the platform's monitoring capabilities by integrating usage tracking for Gemini and Z.ai models, offering users better visibility into their coding plan consumption. It also refines the Claude model experience by upgrading to Opus 4.6 with adaptive thinking and enhances authentication diagnostics. Furthermore, the update includes support for the latest GPT-5.3-Codex model and streamlines the Docker environment for automated testing with Playwright. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds usage tracking for Gemini and Z.ai models, including backend services, API routes, and UI components. The changes are extensive and well-structured. I've identified a few areas for improvement, mainly concerning error handling, performance in synchronous calls, and code duplication in the new backend services. Additionally, there's a minor regression in the UI that should be addressed. Overall, this is a solid feature addition.
| private findGeminiBinaryPath(): string | null { | ||
| try { | ||
| // Try 'which' on Unix-like systems | ||
| const whichResult = execSync('which gemini 2>/dev/null', { encoding: 'utf8' }).trim(); |
There was a problem hiding this comment.
The use of execSync is synchronous and blocks the Node.js event loop, which can degrade server performance. While extractOAuthClientCredentials caches the result, the first call to this endpoint will block. This happens again on line 559. It is recommended to use the asynchronous exec from child_process and make the calling functions (findGeminiBinaryPath and extractOAuthClientCredentials) async to avoid blocking.
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/services/settings-service.ts (1)
728-754:⚠️ Potential issue | 🟠 Major
getMaskedCredentials()does not include the newzaikey.The method returns masked credentials for
anthropic,openaibut omits the newly addedzaikey. This means the UI cannot display whether a z.ai API key is configured.🔧 Proposed fix
async getMaskedCredentials(): Promise<{ anthropic: { configured: boolean; masked: string }; google: { configured: boolean; masked: string }; openai: { configured: boolean; masked: string }; + zai: { configured: boolean; masked: string }; }> { const credentials = await this.getCredentials(); const maskKey = (key: string): string => { if (!key || key.length < 8) return ''; return `${key.substring(0, 4)}...${key.substring(key.length - 4)}`; }; return { anthropic: { configured: !!credentials.apiKeys.anthropic, masked: maskKey(credentials.apiKeys.anthropic), }, google: { configured: !!credentials.apiKeys.google, masked: maskKey(credentials.apiKeys.google), }, openai: { configured: !!credentials.apiKeys.openai, masked: maskKey(credentials.apiKeys.openai), }, + zai: { + configured: !!credentials.apiKeys.zai, + masked: maskKey(credentials.apiKeys.zai), + }, }; }apps/ui/src/components/usage-popover.tsx (1)
30-46:⚠️ Potential issue | 🟡 MinorGuard against past reset times in
formatResetTime.If the reset time is already in the past, the current logic yields negative “Resets in -Xm” text. Add a clamp or fallback message.
🧹 Suggested fix
const now = new Date(); const diff = date.getTime() - now.getTime(); + if (diff <= 0) { + return 'Resetting soon'; + }
🤖 Fix all issues with AI agents
In `@apps/server/src/routes/gemini/index.ts`:
- Around line 35-57: Remove the unsafe (status as any) casts: use the typed
InstallationStatus returned by GeminiProvider.detectInstallation() and compute
authMethod using the existing fields (status.authenticated / status.hasApiKey)
without casting; also remove or type-augment hasCredentialsFile—either add
hasCredentialsFile to the InstallationStatus interface (and populate it in
detectInstallation) or stop returning hasCredentialsFile and default it to false
in the JSON response; update the route in apps/server/src/routes/gemini/index.ts
(references: GeminiProvider, detectInstallation, authMethod, hasCredentialsFile,
InstallationStatus) accordingly.
In `@apps/server/src/routes/zai/index.ts`:
- Around line 63-155: The route handlers for router.post('/configure') and
router.post('/verify') contain business logic
(usageService.setApiToken/setApiHost, settingsService.updateCredentials, fetch
to quota URL and response handling) and must be moved into ZaiUsageService (or a
new service) as methods like configure({apiToken, apiHost}) and
verifyApiKey(apiKey); those service methods should perform the persistence (call
settingsService.updateCredentials), runtime updates (usageService calls),
external fetch, and emit events via the shared emitter from createEventEmitter()
(e.g., emit 'zai.configured' and 'zai.verify.result' with status and error
info). Update the route handlers to simply call ZaiUsageService.configure(...)
and ZaiUsageService.verifyApiKey(...), await results, and return minimal JSON
responses, handling exceptions only to map errors to HTTP responses. Ensure you
reference and replace direct calls to usageService and
settingsService.updateCredentials inside the service and add emitter.emit calls
for UI updates.
- Around line 120-147: The z.ai verification fetch call can hang indefinitely;
update the fetch invocation in the route handler that calls quotaUrl to pass a
signal created via AbortSignal.timeout(timeoutMs) (e.g., const controllerSignal
= AbortSignal.timeout(5000)) in the fetch options so the request auto-aborts
after a reasonable timeout, and enhance the catch block to handle aborts (detect
AbortError / error.name === 'AbortError' and return a clear timeout JSON
response). Ensure you reference the existing fetch call that sets headers
Authorization/Accept and the surrounding try/catch so you only add the signal
option and the abort-specific error branch.
In `@apps/server/src/services/gemini-usage-service.ts`:
- Around line 688-697: The refreshed token is always written to
this.credentialsPath (e.g., in the getValidAccessToken flow) even when
credentials were loaded from a different file (such as
~/.config/gemini/oauth_creds.json), causing divergence; modify the
credential-loading logic (where cachedCredentials is set) to record the actual
source path into a new property (e.g., this.loadedCredentialsPath) and then
change the write path in the save/refresh block to use that recorded
this.loadedCredentialsPath (falling back to this.credentialsPath if not set) so
refreshed tokens are persisted back to the same file they were read from.
- Around line 349-353: The possiblePaths array contains a duplicate because the
constructor already sets this.credentialsPath to the home
'.gemini/oauth_creds.json' path; remove the redundant literal path from
possiblePaths or dedupe the array before use (e.g., filter out duplicates by
keeping only the first occurrence) so possiblePaths only contains unique
entries; update references around possiblePaths and ensure the logic using
possiblePaths still checks the same fallback locations in
GeminiUsageService/constructor.
- Around line 343-384: The cachedCredentials in loadCredentials() is never
invalidated so stale/expired creds persist; modify loadCredentials (and/or
clearCache()) to implement automatic invalidation by adding a TTL (e.g., 5
minutes) or validating the credentials file mtime before returning
cachedCredentials: store a cachedAt timestamp alongside cachedCredentials when
setting it in loadCredentials(), and when loadCredentials is called check if
cachedAt + TTL has expired or if fs.statSync(credPath).mtimeMs is newer than
cachedAt — if expired or file changed, discard cachedCredentials and re-read
from disk (then update cachedAt); ensure clearCache() still exists to allow
explicit invalidation and reference credentialsPath/possiblePaths when comparing
mtimes.
- Around line 557-578: The fallback search in extractOAuthClientCredentials uses
execSync with an interpolated baseDir, creating a command-injection and
quoting/portability risk; change it to only run on POSIX (guard with
process.platform !== 'win32') and replace the string-shell execSync call with a
safe execFileSync/spawnSync invocation that passes 'find' and its arguments as
an array (e.g., args: [ `${baseDir}/..`, '-name', 'oauth2.js', '-path',
'*gemini*', '-path', '*code_assist*' ]) to avoid shell interpolation, keep the
same encoding/timeout options, then proceed to read and pass the result to
parseOAuthCredentialsFromSource and set this.cachedClientCredentials as before.
- Around line 168-202: The external fetch calls in fetchUsageData
(CODE_ASSIST_URL, QUOTA_API_URL, GOOGLE_TOKEN_URL) lack timeouts and can hang;
add an AbortSignal with a timeout for each request (e.g., use
AbortSignal.timeout(timeoutMs) or new AbortController().signal with setTimeout
to abort) and pass the signal option into fetch, catch the resulting
abort/timeout errors and handle/log them similarly to other fetch failures;
ensure each fetch (the code block around CODE_ASSIST_URL, the QUOTA_API_URL
POST, and the GOOGLE_TOKEN_URL call) is updated to create/use a signal and clean
up any controllers if you use AbortController.
In `@apps/server/src/services/zai-usage-service.ts`:
- Around line 165-176: The fetchUsageData method does not emit lifecycle events
for frontend streaming; modify fetchUsageData to call createEventEmitter()
(import from lib/events.ts) at start and emit a "zai.usage.start" event, then
emit "zai.usage.success" with the fetched ZaiUsageData on successful completion,
and emit "zai.usage.error" with error details if any exception occurs (rethrow
after emitting). Use the existing method name fetchUsageData and the same logger
for messages, ensure the emitter is created inside the method and closed/handled
per createEventEmitter's contract so events stream via WebSocket to the
frontend.
- Around line 182-190: The fetch call to quotaUrl currently has no timeout; wrap
it with an AbortController and a timeout like other services (e.g.,
verify-claude-auth.ts/event-hook-service.ts): create an AbortController, start a
setTimeout to call controller.abort() after the configured timeout, pass
controller.signal into the fetch options (where Authorization and Accept headers
are set), and clear the timeout after fetch completes; also handle the abort
error case so the calling function (the code around quotaUrl/token/response)
throws or returns a proper timeout error instead of hanging.
In `@apps/ui/src/components/views/board-view/mobile-usage-bar.tsx`:
- Around line 38-54: The formatResetTime function returns negative "Resets in
-Xm" for past timestamps; update formatResetTime(unixTimestamp, isMilliseconds)
to detect diff <= 0 and return a short message like "Resetting soon" (matching
server-side behavior) before computing minutes/hours/dates, keeping the existing
branches for <1h, <24h and fallback to date; touch the function name
formatResetTime and ensure the diff calculation (const diff = date.getTime() -
now.getTime()) is used to decide the early-return for past/resetting-soon cases.
- Around line 166-167: Add geminiUsage and geminiUsageLastUpdated to the
existing Zustand store alongside the other provider states (the same store that
exposes setClaudeUsage/setCodexUsage/setZaiUsage): add state fields geminiUsage:
GeminiUsage | null and geminiUsageLastUpdated: number | null, and a setter
setGeminiUsage((usage, lastUpdated?) => void) that updates both fields. In
mobile-usage-bar.tsx remove the local useState declarations for geminiUsage and
geminiUsageLastUpdated and replace them with the store selectors/hooks (e.g.,
const { geminiUsage, geminiUsageLastUpdated, setGeminiUsage } = useStore(...))
and update any places that call setGeminiUsage to use the store setter. Ensure
types match the existing GeminiUsage type and that other components (like
UsagePopover) can read the new store fields.
In
`@apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts`:
- Around line 239-259: The code persists the z.ai key to the local store via
setApiKeys before calling api.zai.configure, and uses a loose check
(result.success || result.isAvailable) so the local state can show "Saved" even
if server config failed; either move the setApiKeys call so it runs only after
api.zai.configure succeeds (and then update setZaiAuthStatus and call
queryClient.invalidateQueries), or implement a rollback in the catch where you
remove the stored zai key using the same setApiKeys call; also tighten the
success condition to require result.success (not just isAvailable) when deciding
success for configuring z.ai.
- Around line 246-252: The code in use-api-key-management.ts hardcodes
hasEnvApiKey: false when calling setZaiAuthStatus after saving a UI key, which
can incorrectly clear an existing env-configured key; update the call in the
block that calls setZaiAuthStatus to preserve the current env key flag by
reading the existing apiKeyStatus (e.g., use apiKeyStatus.hasEnvApiKey) and set
hasEnvApiKey to that value instead of false so you only update the fields that
truly change (authenticated, method, hasApiKey) while keeping hasEnvApiKey in
sync.
- Line 1: Remove the top-level "// `@ts-nocheck`" and add explicit TypeScript
typings for the hook and its internal entities: declare interfaces/types such as
ApiKey (id, name, key, createdAt, revoked?), ApiKeyState (array + validation
errors), AuthState (isAuthenticated, userId, token), and ServerConfig
(allowedOrigins, keyPolicy) used by the useApiKeyManagement hook; update
function signatures (useApiKeyManagement, loadApiKeys, saveApiKeys,
validateApiKey, revokeApiKey) and React state hooks to use these types, type any
localStorage/parsing and async calls with proper unions/Optionals, and fix
resulting type errors rather than silencing them so the file compiles with full
type safety.
In `@apps/ui/src/hooks/use-provider-auth-init.ts`:
- Around line 133-138: The code is assigning the wrong field to the CLI path:
update the call to setGeminiCliStatus so the path uses result.path (the actual
filesystem path) instead of result.status; locate the block where
setGeminiCliStatus is called (in use-provider-auth-init.ts) and replace the
path: result.status assignment with path: result.path while keeping installed:
result.installed ?? false and version: result.version unchanged.
- Around line 107-124: The z.ai auth response handler (api.zai.getStatus) can
leave authenticated/hasApiKey undefined; update the logic in the try block that
calls setZaiAuthStatus so it coerces result.available, result.hasApiKey, and
result.hasEnvApiKey to booleans (e.g., using !! or ?? false) and derive method
using those boolean values, and also add a fallback setZaiAuthStatus in the
non-success/exception path to set authenticated: false, method: 'none',
hasApiKey: false, hasEnvApiKey: false; reference setZaiAuthStatus,
api.zai.getStatus, and ZaiAuthMethod when making the changes.
🧹 Nitpick comments (6)
apps/ui/src/store/app-store.ts (1)
97-105: Consider hoisting Zai/Gemini usage types into@automaker/types.These usage shapes now exist in both server services and the UI store; centralizing them avoids drift and simplifies cross-layer contracts.
Based on learnings, “Core TypeScript definitions with no dependencies should be isolated in the automaker/types package”.Also applies to: 184-191
apps/ui/src/store/types/usage-types.ts (3)
65-66:limitType: 'TOKENS_LIMIT' | 'TIME_LIMIT' | string— the literal union members are redundant.Since
stringis a supertype of all string literals,'TOKENS_LIMIT' | 'TIME_LIMIT' | stringcollapses to juststringat the type level. If the intent is to provide autocomplete hints while still allowing arbitrary strings, use the(string & {})idiom instead.✏️ Suggested fix
export interface ZaiQuotaLimit { - limitType: 'TOKENS_LIMIT' | 'TIME_LIMIT' | string; + limitType: 'TOKENS_LIMIT' | 'TIME_LIMIT' | (string & {}); limit: number;
108-133:authMethodis typed asstringbut documented with known values — prefer a string literal union.The JSDoc on Line 111 lists
'cli_login' | 'api_key' | 'api_key_env' | 'none'but the actual type isstring. This weakens type safety for consumers (e.g., the ternary chains inmobile-usage-bar.tsxLines 407-411 comparing against literal strings). Using a union (with a(string & {})escape hatch if needed) would catch typos at compile time.✏️ Suggested fix
+export type GeminiAuthMethod = 'cli_login' | 'api_key' | 'api_key_env' | 'none'; + export interface GeminiUsage { /** Whether the user is authenticated (via CLI or API key) */ authenticated: boolean; - /** Authentication method: 'cli_login' | 'api_key' | 'api_key_env' | 'none' */ - authMethod: string; + /** Authentication method */ + authMethod: GeminiAuthMethod | (string & {});
62-84: Move API response types to@automaker/typesand standardize type definitions.
ZaiPlanType,ZaiQuotaLimit,ZaiUsage,GeminiQuotaBucket,GeminiTierQuota, andGeminiUsageare duplicated across frontend (apps/ui/src/store/types/usage-types.ts) and backend services (apps/server/src/services/). Since these are core type definitions with no app-specific dependencies, they should be defined once in@automaker/typesand imported by both frontend and backend.Additionally, align type safety for
GeminiUsage.authMethod: use a literal union type ('cli_login' | 'api_key' | 'none') instead ofstringto match the backend definition and prevent type mismatches. The JSDoc mentions'api_key_env', which is not present in the backend's type—clarify whether this is needed or remove it.apps/ui/src/components/views/board-view/mobile-usage-bar.tsx (2)
375-425: Gemini usage rendering handles multiple states clearly.The nested ternary covers: authenticated with tier quotas → authenticated without quotas (shows auth method + error/no-usage) → not authenticated → loading. The logic is sound, though the deeply nested ternaries could be extracted to a sub-component for readability in a follow-up.
24-54: ExtractformatNumberandformatResetTimeto a shared utility module.These helpers are duplicated identically in
usage-popover.tsxandmobile-usage-bar.tsx. Extract them to@/lib/format-utils.tsto eliminate duplication and improve maintainability across desktop (UsagePopover) and mobile (MobileUsageBar) usage-display components.Note:
codex-usage-popover.tsxhas a separate, simplerformatResetTimeimplementation for Codex-specific formatting that should remain separate.
apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts
Show resolved
Hide resolved
apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts
Show resolved
Hide resolved
…sage-tracking # Conflicts: # apps/ui/src/components/usage-popover.tsx # apps/ui/src/components/views/board-view/mobile-usage-bar.tsx
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/server/src/routes/models/routes/providers.ts (1)
15-27:⚠️ Potential issue | 🟠 MajorAdd missing provider entries to the
providersresponse map.The endpoint fetches statuses for all registered providers (claude, cursor, codex, opencode, gemini, copilot) via
checkAllProviders(), but only returns anthropic and cursor to the frontend. This means gemini and other providers cannot be discovered via the/providersendpoint. Either expand the response to include all fetched providers or document why certain providers are intentionally excluded.Note: Z.ai is not in the provider registry and has a separate route, so it's not affected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/models/routes/providers.ts` around lines 15 - 27, The providers response currently only returns "anthropic" and "cursor" from the statuses map; update the providers Record in apps/server/src/routes/models/routes/providers.ts (the providers variable created from checkAllProviders()'s statuses) to include all registered providers returned by checkAllProviders() — e.g., claude, cursor, codex, opencode, gemini, copilot (and any others your registry returns) with appropriate fields (available, version, path, method, authenticated and hasApiKey where relevant); also correct the mapping for "anthropic" if it mistakenly reads statuses.claude (ensure anthropic reads statuses.anthropic) so the frontend can discover all providers via the /providers endpoint.apps/server/src/routes/app-spec/common.ts (1)
50-55:⚠️ Potential issue | 🟡 MinorPre-existing logic bug:
if (running)on aRunningGenerationobject is always truthy.
runningis typed asRunningGeneration(an object). The truthiness check never looks atrunning.isRunning, so the function returns the first entry in the map rather than the first running entry. It works today only becausesetRunningStatedeletes entries onrunning=false, keeping the map as an implicit "all running" set — but that's a fragile invariant.🐛 Proposed fix
export function getRunningProjectPath(): string | null { for (const [path, running] of runningProjects.entries()) { - if (running) return path; + if (running.isRunning) return path; } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/app-spec/common.ts` around lines 50 - 55, getRunningProjectPath currently checks the truthiness of the map value (RunningGeneration) so it always returns the first map entry; update it to inspect the actual running flag (e.g., running.isRunning) when iterating runningProjects so it only returns the path for entries where RunningGeneration.isRunning is true; ensure you reference the RunningGeneration shape and preserve behavior of setRunningState (which may delete entries) so this becomes a correct explicit check rather than relying on that invariant.apps/server/src/providers/codex-provider.ts (1)
769-812:⚠️ Potential issue | 🟠 Major
buildConfigOverrides(overrides)return value is discarded — CLI config settings are never applied.
buildConfigOverridesis a pure function that returnsstring[]of--config key=valueflags. The returned array is not stored or spread intoargs, so none of the built overrides (max_turns,reasoning_effort,approval_policy,features.web_search_request) reach the Codex CLI. This silently regresses:
max_turns– the CLI may run indefinitely.reasoning_effort– the model ignores the configured effort level;calculateReasoningTimeout(line 829) computes a correct timeout, but the effort itself is never passed to the CLI.features.web_search_request– web search is always disabled regardless of settings.The entire
overridesbuild-up (lines 769–790) plus thebuildConfigOverridescall are effectively dead code in the current state. Either restore the spread intoargsor remove the dead code entirely if the CLI no longer accepts--configflags.🐛 Proposed fix (restore config args)
- buildConfigOverrides(overrides); - const preExecArgs: string[] = []; + const configArgs = buildConfigOverrides(overrides); + const preExecArgs: string[] = [...configArgs];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/codex-provider.ts` around lines 769 - 812, The buildConfigOverrides(overrides) return value is unused so the CLI never receives the generated --config flags; capture its returned string[] and include (spread) it into the args array before executing the Codex CLI. Specifically, after building overrides (the overrides array) call buildConfigOverrides(overrides), assign the result to a variable (e.g., configArgs) and insert/spread those values into the args construction (with CODEX_EXEC_SUBCOMMAND, CODEX_YOLO_FLAG..., ...preExecArgs, CODEX_MODEL_FLAG, options.model, CODEX_JSON_FLAG, '-') so that max_turns, CODEX_REASONING_EFFORT_KEY, approval_policy and features.web_search_request are passed to the CLI; alternatively, if the CLI no longer accepts --config flags, remove the overrides building block (overrides and buildConfigOverrides usage) and associated constants to avoid dead code.apps/server/src/services/recovery-service.ts (1)
297-329:⚠️ Potential issue | 🟡 Minor
clearExecutionStateis never called on the empty-list early return.Line 298's
returnexits the method before reachingclearExecutionStateon line 328. IfreconcileAllFeatureStates()already moved all previously-running features to terminal states beforeresumeInterruptedFeaturesis called,allInterruptedFeatureswill be empty and the execution-state file will persist on disk indefinitely. Every subsequent call hits the same early return, so the file is never cleaned up.🛠️ Proposed fix
const allInterruptedFeatures = [...featuresWithContext, ...featuresWithoutContext]; - if (allInterruptedFeatures.length === 0) return; + if (allInterruptedFeatures.length === 0) { + await this.clearExecutionState(projectPath); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/recovery-service.ts` around lines 297 - 329, The early return in resumeInterruptedFeatures exits when allInterruptedFeatures is empty and never calls clearExecutionState, leaving the execution-state file on disk; update resumeInterruptedFeatures so that when allInterruptedFeatures.length === 0 it still awaits this.clearExecutionState(projectPath) before returning (or move the clearExecutionState call earlier, e.g., immediately after computing allInterruptedFeatures and before the return), ensuring clearExecutionState(projectPath) is always invoked even on the empty-list path; keep references to the allInterruptedFeatures variable and clearExecutionState method in your change so reviewers can easily locate the fix.
🧹 Nitpick comments (14)
apps/server/src/routes/app-spec/generate-features-from-spec.ts (1)
29-33: Remove the dead constant entirely rather than suppressing the lint warning with_.The constant
_CODEX_FEATURE_GENERATION_TIMEOUT_MSis never referenced anywhere in this file — the actual Codex timeout extension is handled by settingeffectiveReasoningEffort = 'xhigh'at Line 206. The_prefix only silences the linter; the dead code (including the JSDoc block) remains. Given the PR's stated goal of removing unused variables, deleting both the comment and the constant is the right call.♻️ Proposed removal
-/** - * Timeout for Codex models when generating features (5 minutes). - * Codex models are slower and need more time to generate 50+ features. - */ -const _CODEX_FEATURE_GENERATION_TIMEOUT_MS = 300000; // 5 minutes -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/app-spec/generate-features-from-spec.ts` around lines 29 - 33, Remove the unused JSDoc and constant _CODEX_FEATURE_GENERATION_TIMEOUT_MS from the file; locate the declaration of the constant named _CODEX_FEATURE_GENERATION_TIMEOUT_MS and delete both its comment block and the const line, leaving the rest of the file (including the existing Codex handling via effectiveReasoningEffort = 'xhigh') unchanged.apps/server/src/tests/cli-integration.test.ts (1)
273-278: Correct use of the_prefix for the unused destructured key.
_providerfollows the idiomatic TypeScript/ESLint convention for intentionally unused destructured parameters, cleanly suppressing the no-unused-vars warning without removing the structural destructure.One pre-existing nit adjacent to this change: the
expect(result.issues.length).toBeGreaterThan(0)assertion on line 275 is tautological — the enclosingifguard already confirmsresult.issues.length > 0. Consider simplifying to just the meaningful assertion:♻️ Proposed simplification of the loop body
Object.entries(results).forEach(([_provider, result]) => { if (!result.detected && result.issues.length > 0) { - expect(result.issues.length).toBeGreaterThan(0); expect(result.issues[0]).toBeTruthy(); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/tests/cli-integration.test.ts` around lines 273 - 278, The loop currently uses the unused destructured key name _provider correctly, but the inner assertion expect(result.issues.length).toBeGreaterThan(0) is tautological because the if guard already checks result.issues.length > 0; remove that redundant assertion and keep only the meaningful check (e.g., assert the first issue is truthy) inside the for-each over Object.entries(results). Maintain the destructured parameter name _provider and the variable result to locate the loop and update its body accordingly.apps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsx (2)
129-166: Duplicated Logs + Stop button JSX across branches.The Logs button (lines 130–152) and Stop button (lines 154–166) are near-identical copies of lines 80–102 and 104–117 in the
isCurrentAutoTaskblock above. Consider extracting small helper components (e.g.,LogsButtonandStopButton) to reduce the duplication and keep future styling/behavior changes in sync.♻️ Sketch of extracted helpers
+// At module level or above the component +function LogsButton({ featureId, shortcutKey, onViewOutput }: { + featureId: string; shortcutKey?: string; onViewOutput: () => void; +}) { + return ( + <Button + variant="secondary" + size="sm" + className="flex-1 h-7 text-[11px]" + onClick={(e) => { e.stopPropagation(); onViewOutput(); }} + onPointerDown={(e) => e.stopPropagation()} + data-testid={`view-output-${featureId}`} + > + <FileText className="w-3 h-3 mr-1 shrink-0" /> + <span className="truncate">Logs</span> + {shortcutKey && ( + <span className="ml-1.5 px-1 py-0.5 text-[9px] font-mono rounded bg-foreground/10" + data-testid={`shortcut-key-${featureId}`}> + {shortcutKey} + </span> + )} + </Button> + ); +} + +function StopButton({ featureId, onForceStop }: { + featureId: string; onForceStop: () => void; +}) { + return ( + <Button + variant="destructive" + size="sm" + className="h-7 text-[11px] px-2 shrink-0" + onClick={(e) => { e.stopPropagation(); onForceStop(); }} + onPointerDown={(e) => e.stopPropagation()} + data-testid={`force-stop-${featureId}`} + > + <StopCircle className="w-3 h-3" /> + </Button> + ); +}Then replace the four occurrences (two in
isCurrentAutoTask, two in the race-window branch) with<LogsButton … />and<StopButton … />.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsx` around lines 129 - 166, Duplicate JSX for the Logs and Stop buttons appears in both the isCurrentAutoTask branch and the alternate branch; extract these into two small stateless components (e.g., LogsButton and StopButton) that accept the needed props (onViewOutput, onForceStop, shortcutKey, feature.id) and preserve the existing handlers (e.stopPropagation in onClick and onPointerDown), testids (`view-output-${feature.id}`, `shortcut-key-${feature.id}`, `force-stop-${feature.id}`) and classes; then replace the four duplicated blocks with <LogsButton .../> and <StopButton .../> calls so styling/behavior stays centralized and future updates are applied in one place (ensure prop names match the existing onViewOutput/onForceStop handlers).
1-1:@ts-nochecksuppresses all type checking for this file.This was pre-existing, but given that the file is growing in complexity with new conditional branches, it would be worth revisiting whether proper discriminated-union typing on the props (e.g., using
feature.statusas the discriminant) could replace this blanket suppression. The comment mentions "optional callback prop typing with feature status narrowing" — TypeScript's narrowing and overloaded prop interfaces can handle this well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsx` at line 1, The file currently uses a blanket "@ts-nocheck" which hides real typing issues; remove that and replace it with a discriminated-union props type that uses feature.status as the discriminant so TypeScript can narrow allowed optional callbacks per state. Update the CardActions component props (e.g., the Props / CardActionsProps interface and any Feature type) to declare unions like { feature: { status: "open"; ... }; onOpenAction?: (...) => void } | { feature: { status: "closed"; ... }; onCloseAction?: (...) => void } (adjust callback names to match existing callbacks in this file) so inside CardActions you can switch/if on feature.status and call only the callbacks valid for that branch without type errors; also update any usages to satisfy the new prop shape. Ensure you remove "@ts-nocheck" after types compile.apps/server/src/routes/features/routes/generate-title.ts (1)
17-20: Remove the unusedprojectPathfield fromGenerateTitleRequestBody.After line 37 stopped destructuring
projectPath, the field remains dead code in the interface. Removing it prevents misleading API consumers that the field is consumed.Cleanup
interface GenerateTitleRequestBody { description: string; - projectPath?: string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/features/routes/generate-title.ts` around lines 17 - 20, The interface GenerateTitleRequestBody currently declares an unused optional field projectPath; remove the projectPath property from the GenerateTitleRequestBody interface so the type only contains description: string, and search for/remediate any remaining references to GenerateTitleRequestBody.projectPath (e.g., in request destructuring or docs) to avoid stale or misleading API typings.apps/server/src/routes/models/routes/providers.ts (1)
9-34: Consider delegating to a service and emitting provider-check events.Two guideline points apply here:
- The route handler calls
ProviderFactory.checkAllProviders()directly rather than delegating to a dedicated service inservices/.- No event is emitted via
createEventEmitter()to stream the result to the frontend via WebSocket.Both are low-impact for a read-only status check, but they diverge from the project conventions.
As per coding guidelines: "Server business logic should be organized into services in the services/ directory, with Express route handlers in routes/ that delegate to services" and "All server operations should emit events using createEventEmitter() from lib/events.ts that stream to the frontend via WebSocket."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/models/routes/providers.ts` around lines 9 - 34, The route handler createProvidersHandler should delegate provider-check logic to a new service function (e.g., services/providersService.getAllProvidersStatus) instead of calling ProviderFactory.checkAllProviders() directly; move the call to ProviderFactory.checkAllProviders() into that service, build the providers object there, and have the service emit the result via createEventEmitter() (from lib/events.ts) so the status is streamed to the frontend; then update createProvidersHandler to call getAllProvidersStatus(), return its result in the JSON response, and handle errors as before.apps/server/src/lib/cli-detection.ts (1)
24-28: Remove dead API surface:includeWslandwslDistributionfromCliDetectionOptions.These fields are defined in the interface but never destructured or used in
detectCliordetectAllCLis. No callers pass these options, and removing them will eliminate misleading API surface.♻️ Proposed cleanup
export interface CliDetectionOptions { timeout?: number; - includeWsl?: boolean; - wslDistribution?: string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/cli-detection.ts` around lines 24 - 28, The CliDetectionOptions interface exposes unused properties includeWsl and wslDistribution which are dead API; remove these two fields from the CliDetectionOptions declaration and update any related docs/comments to reflect the reduced shape, and verify there are no references in detectCli or detectAllCLis (and remove any unused destructuring or parameter defaults there if present) so the public API only includes timeout as used by the implementation.apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx (1)
115-123: Consider extractingisInExecutionStateinto a shared helper.The
isInExecutionState+isActivelyRunningcomputation is duplicated verbatim in bothkanban-card.tsxandlist-view/list-row.tsx. Extracting it into a shared utility (e.g., in@/lib/feature-utils.ts) would keep the guard logic in one place and make future status additions automatic for both views.♻️ Suggested extraction
// `@/lib/feature-utils.ts` (new or existing utils file) +export function isFeatureInExecutionState(status: string): boolean { + return status === 'in_progress' || status.startsWith('pipeline_'); +}Then in both components:
-const isInExecutionState = - feature.status === 'in_progress' || - (typeof feature.status === 'string' && feature.status.startsWith('pipeline_')); -const isActivelyRunning = !!isCurrentAutoTask && isInExecutionState; +const isActivelyRunning = !!isCurrentAutoTask && isFeatureInExecutionState(feature.status);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx` around lines 115 - 123, Extract the duplicated execution-state logic into a single exported helper (e.g., isFeatureInExecutionState) and replace the inline `isInExecutionState` checks in both the Kanban card and List row with a call to that helper; specifically, create a function (exported from a shared utils module) that accepts the feature status string and returns true for `status === 'in_progress' || status.startsWith('pipeline_')`, then update the `isInExecutionState` usage in the kanban-card component (where `isActivelyRunning` is computed) and the corresponding `list-row` computation to call this helper so the guard logic is centralized and reused.apps/ui/src/components/views/board-view/components/list-view/row-actions.tsx (2)
489-499: Redundanthandlers.onForceStopguard inside a block that already requires it.The outer condition at line 464 already ensures
handlers.onForceStopis truthy when entering this branch, making the inner{handlers.onForceStop && ...}check at line 489 unnecessary.♻️ Suggested simplification
- {handlers.onForceStop && ( - <> - <DropdownMenuSeparator /> - <MenuItem - icon={StopCircle} - label="Force Stop" - onClick={withClose(handlers.onForceStop)} - variant="destructive" - /> - </> - )} + <> + <DropdownMenuSeparator /> + <MenuItem + icon={StopCircle} + label="Force Stop" + onClick={withClose(handlers.onForceStop)} + variant="destructive" + /> + </>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/list-view/row-actions.tsx` around lines 489 - 499, The inner guard `{handlers.onForceStop && (...)}` is redundant because the outer branch already ensures `handlers.onForceStop` is truthy; remove the inner conditional and directly render the Separator and MenuItem (using StopCircle, MenuItem, DropdownMenuSeparator and onClick={withClose(handlers.onForceStop)}) to simplify the JSX and avoid duplicate checks.
503-507: Simplify the double-negation condition for clarity.
!(!feature.error && handlers.onForceStop)is logically equivalent to(feature.error || !handlers.onForceStop)(De Morgan's law) but harder to parse at a glance.♻️ Suggested simplification
- !(!feature.error && handlers.onForceStop) && ( + (!!feature.error || !handlers.onForceStop) && (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/list-view/row-actions.tsx` around lines 503 - 507, The conditional rendering uses a confusing double-negation: replace the expression "!(!feature.error && handlers.onForceStop)" with the clearer equivalent "(feature.error || !handlers.onForceStop)" so the full condition becomes: !isCurrentAutoTask && feature.status === 'in_progress' && (feature.error || !handlers.onForceStop); update the condition where it's used in row-actions.tsx (around the block guarded by isCurrentAutoTask, feature.status, feature.error, and handlers.onForceStop) to improve readability while preserving logic.apps/server/src/providers/codex-provider.ts (1)
1020-1025:getCodexAuthIndicators()is called but its result is unused — deadawait.
detectInstallation()now derives auth status exclusively fromcheckCodexAuthentication()(line 1041). Theawait getCodexAuthIndicators()on line 1023 is a pure read with no side effects; discarding its result makes the call a no-op. Remove it to eliminate unnecessary I/O on everydetectInstallationinvocation.🛠️ Proposed fix
- await getCodexAuthIndicators(); const installed = !!cliPath;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/codex-provider.ts` around lines 1020 - 1025, The call to getCodexAuthIndicators() inside detectInstallation() is unused and creates unnecessary I/O; remove the dead await call and let detectInstallation() rely on hasApiKey and checkCodexAuthentication() for auth state instead (update any docs/comments if present). Specifically, delete the await getCodexAuthIndicators() invocation from the detectInstallation function and ensure checkCodexAuthentication() (and hasApiKey) remain the source of authentication status.apps/server/src/services/zai-usage-service.ts (1)
349-383: RedundantclearTimeoutinsidetry—finallyalready covers it.
clearTimeout(timeoutId)is called at line 363 inside thetryblock and again unconditionally in thefinallyblock at line 382. Thefinallycall subsumes line 363; callingclearTimeouton an already-cleared timer is harmless but the inner call is unnecessary noise.♻️ Suggested cleanup
const response = await fetch(quotaUrl, { ... }); - clearTimeout(timeoutId); if (!response.ok) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/zai-usage-service.ts` around lines 349 - 383, The code calls clearTimeout(timeoutId) twice inside fetchUsageData: once inside the inner try after await fetch and again in the finally block; remove the redundant inner clearTimeout call (the finally block that clears timeoutId using the AbortController/timeoutId already guarantees cleanup) so only the finally retains clearTimeout(timeoutId) and the AbortController usage remains unchanged.apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts (1)
10-11:importappears after a non-import statement — move it to the top.Line 11's
import { getElectronAPI }is placed after theconst logger = createLogger(...)statement on line 10. ES/TS imports are hoisted at runtime, but this violatesimport/firstlint rules and is non-idiomatic. All imports should precede any executable module-level statements.♻️ Proposed fix
+import { getElectronAPI } from '@/lib/electron'; +import type { ProviderConfigParams } from '@/config/api-providers'; + const logger = createLogger('ApiKeyManagement'); -import { getElectronAPI } from '@/lib/electron'; -import type { ProviderConfigParams } from '@/config/api-providers';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts` around lines 10 - 11, The module-level `import { getElectronAPI }` is placed after the executable statement `const logger = createLogger('ApiKeyManagement')`; move the `getElectronAPI` import (and any other imports) above any executable code so all imports appear at the top of the file, e.g. place `import { getElectronAPI } from '@/lib/electron';` before the `const logger = createLogger('ApiKeyManagement');` line and group it with the other imports to satisfy the `import/first` lint rule.apps/server/src/services/gemini-usage-service.ts (1)
247-299:Array.map()used with external side-effects — considerforEach+ separate return.The
mapcallback on line 260 both builds the returnedGeminiQuotaBucket[]and mutates six external variables (overallLowestRemaining,constrainedModel,overallResetTime,flashLowestRemaining, etc.). Usingmapfor side-effects is non-idiomatic and makes the data-flow harder to follow. AforEachfor the tracking pass followed by a separatemap(or combined in areduce) would make intent clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/gemini-usage-service.ts` around lines 247 - 299, The current use of apiBuckets.map in the Gemini quota computation mixes returning a GeminiQuotaBucket[] with mutating external tracking variables (overallLowestRemaining, constrainedModel, overallResetTime, flashLowestRemaining, flashResetTime, hasFlashModels, pro equivalents), which is non-idiomatic; refactor by first iterating over apiBuckets with a forEach (or a single reduce) to compute the tracking variables (update overallLowestRemaining, constrainedModel, overallResetTime, hasFlashModels, flashLowestRemaining, flashResetTime, hasProModels, proLowestRemaining, proResetTime) and then create the quotaBuckets array with a pure map that only returns { modelId, remainingFraction, resetTime } (or do the whole thing in one reduce that returns both the tracking summary and the array), updating code in gemini-usage-service.ts around the apiBuckets.map usage and referencing the same variable names used now.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/lib/permission-enforcer.ts`:
- Around line 15-20: The CursorToolCall interface is currently unexported but
used by exported functions checkToolCallPermission and logPermissionViolation;
either export CursorToolCall from this file now or (preferred) move its minimal
definition into `@automaker/types` and import it here (aligning with
CursorToolCallEvent already in that package). Update the local file to import
the moved CursorToolCall and adjust any references in checkToolCallPermission
and logPermissionViolation to use the shared type; keep the current all-optional
shape for compatibility and do not change the function logic.
In `@apps/server/src/providers/cursor-provider.ts`:
- Line 882: The call to getEffectivePermissions(...) inside executeQuery is dead
I/O because its return value is discarded; either remove the call and its import
(if permissions are not used) or use the returned permissions to enforce checks
before proceeding. If removing, delete the getEffectivePermissions call and
remove its import; if enforcing, assign const permissions = await
getEffectivePermissions(options.cwd || process.cwd()) and use permissions to
validate/deny file writes or shell command spawning in executeQuery (e.g., gate
the subprocess spawn or write operations based on permissions).
In `@apps/server/src/routes/setup/routes/verify-claude-auth.ts`:
- Line 154: The temp env override created by createTempEnvOverride(authEnv) is
never invoked (stored as _cleanupEnv) so process.env retains injected
credentials; ensure you call the returned cleanup function in the verification
flow's finally block: store the result of createTempEnvOverride(authEnv) (e.g.,
const cleanupEnv = createTempEnvOverride(authEnv)) and invoke cleanupEnv()
inside the existing finally block in verify-claude-auth (or wrap the try block
that uses the temp env), guaranteeing the restore runs regardless of success or
error to remove ANTHROPIC_API_KEY/ANTHROPIC_BASE_URL from process.env.
In `@apps/server/src/services/gemini-usage-service.ts`:
- Around line 449-470: The findGeminiBinaryPath implementation currently returns
directory paths because possiblePaths includes directory roots (e.g.,
path.join(os.homedir(), '.nvm', 'versions', 'node') and path.join(os.homedir(),
'.fnm', 'node-versions')), causing extractOAuthClientCredentials to compute the
wrong baseDir and fail OAuth discovery; remove those non-binary directory
entries from possiblePaths (or replace them with explicit versioned binary
paths) so only actual gemini executable paths are checked, or alternatively
change the existence check to confirm the path is a file/executable before
returning (update findGeminiBinaryPath and ensure extractOAuthClientCredentials
still derives baseDir from a confirmed binary path).
In `@apps/server/src/services/zai-usage-service.ts`:
- Around line 189-230: The methods configure, verifyApiKey and fetchUsageData
currently call createEventEmitter() locally which produces a new emitter with no
subscribers, so emitter.emit(...) are no-ops; modify the ZaiUsageService to
accept a shared TypedEventBus (or the shared emitter) via constructor injection
(e.g., constructor(private eventBus: TypedEventBus) {}) and replace local
createEventEmitter() usages with this.eventBus.emit(...) (referencing configure,
verifyApiKey, fetchUsageData and any other methods using createEventEmitter());
ensure the constructor parameter is used throughout and remove the local
createEventEmitter() calls so emitted 'notification:created' events are
delivered to subscribers.
- Around line 172-175: getApiHost() unconditionally prefixes Z_AI_API_HOST with
"https://" which yields double-protocol strings if the env var already contains
a scheme; change the logic so when process.env.Z_AI_API_HOST is present you
first check if it startsWith('http') and return it as-is, otherwise prepend
'https://'; apply the identical pattern to the inline usage of
process.env.Z_AI_API_HOST in verifyApiKey() so both functions (getApiHost and
verifyApiKey) consistently handle full URLs and bare hostnames.
In `@apps/ui/src/components/views/board-view/mobile-usage-bar.tsx`:
- Around line 237-251: fetchZaiUsage currently calls setZaiUsage(data) without a
timestamp so zaiUsageLastUpdated may never be set; mirror fetchGeminiUsage's
behavior by passing a timestamp to the store setter (e.g., call
setZaiUsage(data, Date.now())), and update the setZaiUsage type/signature where
it’s defined to accept the optional/required timestamp (similar to
setGeminiUsage) so the stale-check using zaiUsageLastUpdated works and z.ai
usage is cached for the 2-minute window.
In
`@apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts`:
- Around line 228-251: The code calls getElectronAPI() without guarding for a
null return, so api.zai?.verify can throw; update the verify flow in the
function that contains getElectronAPI() (the block using api.zai?.verify and
calling setZaiTestResult/setTestingZaiConnection) to first check api is present
(e.g., if (!api || !api.zai || typeof api.zai.verify !== 'function') ), and if
missing setZaiTestResult to a clear error about the desktop API not being
available and skip calling verify; keep the existing catch/finally behavior but
ensure you return early when the API is absent to avoid the TypeError.
---
Outside diff comments:
In `@apps/server/src/providers/codex-provider.ts`:
- Around line 769-812: The buildConfigOverrides(overrides) return value is
unused so the CLI never receives the generated --config flags; capture its
returned string[] and include (spread) it into the args array before executing
the Codex CLI. Specifically, after building overrides (the overrides array) call
buildConfigOverrides(overrides), assign the result to a variable (e.g.,
configArgs) and insert/spread those values into the args construction (with
CODEX_EXEC_SUBCOMMAND, CODEX_YOLO_FLAG..., ...preExecArgs, CODEX_MODEL_FLAG,
options.model, CODEX_JSON_FLAG, '-') so that max_turns,
CODEX_REASONING_EFFORT_KEY, approval_policy and features.web_search_request are
passed to the CLI; alternatively, if the CLI no longer accepts --config flags,
remove the overrides building block (overrides and buildConfigOverrides usage)
and associated constants to avoid dead code.
In `@apps/server/src/routes/app-spec/common.ts`:
- Around line 50-55: getRunningProjectPath currently checks the truthiness of
the map value (RunningGeneration) so it always returns the first map entry;
update it to inspect the actual running flag (e.g., running.isRunning) when
iterating runningProjects so it only returns the path for entries where
RunningGeneration.isRunning is true; ensure you reference the RunningGeneration
shape and preserve behavior of setRunningState (which may delete entries) so
this becomes a correct explicit check rather than relying on that invariant.
In `@apps/server/src/routes/models/routes/providers.ts`:
- Around line 15-27: The providers response currently only returns "anthropic"
and "cursor" from the statuses map; update the providers Record in
apps/server/src/routes/models/routes/providers.ts (the providers variable
created from checkAllProviders()'s statuses) to include all registered providers
returned by checkAllProviders() — e.g., claude, cursor, codex, opencode, gemini,
copilot (and any others your registry returns) with appropriate fields
(available, version, path, method, authenticated and hasApiKey where relevant);
also correct the mapping for "anthropic" if it mistakenly reads statuses.claude
(ensure anthropic reads statuses.anthropic) so the frontend can discover all
providers via the /providers endpoint.
In `@apps/server/src/services/recovery-service.ts`:
- Around line 297-329: The early return in resumeInterruptedFeatures exits when
allInterruptedFeatures is empty and never calls clearExecutionState, leaving the
execution-state file on disk; update resumeInterruptedFeatures so that when
allInterruptedFeatures.length === 0 it still awaits
this.clearExecutionState(projectPath) before returning (or move the
clearExecutionState call earlier, e.g., immediately after computing
allInterruptedFeatures and before the return), ensuring
clearExecutionState(projectPath) is always invoked even on the empty-list path;
keep references to the allInterruptedFeatures variable and clearExecutionState
method in your change so reviewers can easily locate the fix.
---
Duplicate comments:
In `@apps/ui/src/components/views/board-view/mobile-usage-bar.tsx`:
- Around line 38-59: formatResetTime now correctly guards against past
timestamps by returning 'Resetting soon' when diff <= 0; mark the earlier review
thread as resolved and remove any duplicate/commented-out legacy implementations
related to formatResetTime to avoid confusion. Locate the helper function
formatResetTime in mobile-usage-bar.tsx and delete any redundant duplicate code
blocks or commented alternatives, then resolve the review conversation so only
the updated implementation remains.
- Around line 188-191: The code currently calls useAppStore() four times to pull
claudeUsage, codexUsage, zaiUsage and geminiUsage (and their
LastUpdated/setters); consolidate these into a single destructuring call to
useAppStore() to return { claudeUsage, claudeUsageLastUpdated, setClaudeUsage,
codexUsage, codexUsageLastUpdated, setCodexUsage, zaiUsage, zaiUsageLastUpdated,
setZaiUsage, geminiUsage, geminiUsageLastUpdated, setGeminiUsage } so the
component subscribes once and avoids duplicate hooks/subscriptions; update the
references in mobile-usage-bar.tsx to use that single destructured object and
remove the multiple useAppStore() calls.
In
`@apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts`:
- Around line 284-303: The failure and catch branches call setApiKeys with zai:
zaiKey which persists a z.ai key locally even though api.zai.configure() failed;
change those branches (where result.success is false and in the catch) to avoid
persisting the failed zaiKey by either omitting zai from the payload and using
the existing apiKeys.zai value or explicitly setting zai: '' (depending on
desired UX), i.e., update the setApiKeys calls inside the error branch and catch
block so they do not write zai: zaiKey and instead reference apiKeys.zai or ''
while leaving anthropic/google/openai as-is; also keep or improve the
logger.error calls around api.zai.configure for clarity.
---
Nitpick comments:
In `@apps/server/src/lib/cli-detection.ts`:
- Around line 24-28: The CliDetectionOptions interface exposes unused properties
includeWsl and wslDistribution which are dead API; remove these two fields from
the CliDetectionOptions declaration and update any related docs/comments to
reflect the reduced shape, and verify there are no references in detectCli or
detectAllCLis (and remove any unused destructuring or parameter defaults there
if present) so the public API only includes timeout as used by the
implementation.
In `@apps/server/src/providers/codex-provider.ts`:
- Around line 1020-1025: The call to getCodexAuthIndicators() inside
detectInstallation() is unused and creates unnecessary I/O; remove the dead
await call and let detectInstallation() rely on hasApiKey and
checkCodexAuthentication() for auth state instead (update any docs/comments if
present). Specifically, delete the await getCodexAuthIndicators() invocation
from the detectInstallation function and ensure checkCodexAuthentication() (and
hasApiKey) remain the source of authentication status.
In `@apps/server/src/routes/app-spec/generate-features-from-spec.ts`:
- Around line 29-33: Remove the unused JSDoc and constant
_CODEX_FEATURE_GENERATION_TIMEOUT_MS from the file; locate the declaration of
the constant named _CODEX_FEATURE_GENERATION_TIMEOUT_MS and delete both its
comment block and the const line, leaving the rest of the file (including the
existing Codex handling via effectiveReasoningEffort = 'xhigh') unchanged.
In `@apps/server/src/routes/features/routes/generate-title.ts`:
- Around line 17-20: The interface GenerateTitleRequestBody currently declares
an unused optional field projectPath; remove the projectPath property from the
GenerateTitleRequestBody interface so the type only contains description:
string, and search for/remediate any remaining references to
GenerateTitleRequestBody.projectPath (e.g., in request destructuring or docs) to
avoid stale or misleading API typings.
In `@apps/server/src/routes/models/routes/providers.ts`:
- Around line 9-34: The route handler createProvidersHandler should delegate
provider-check logic to a new service function (e.g.,
services/providersService.getAllProvidersStatus) instead of calling
ProviderFactory.checkAllProviders() directly; move the call to
ProviderFactory.checkAllProviders() into that service, build the providers
object there, and have the service emit the result via createEventEmitter()
(from lib/events.ts) so the status is streamed to the frontend; then update
createProvidersHandler to call getAllProvidersStatus(), return its result in the
JSON response, and handle errors as before.
In `@apps/server/src/services/gemini-usage-service.ts`:
- Around line 247-299: The current use of apiBuckets.map in the Gemini quota
computation mixes returning a GeminiQuotaBucket[] with mutating external
tracking variables (overallLowestRemaining, constrainedModel, overallResetTime,
flashLowestRemaining, flashResetTime, hasFlashModels, pro equivalents), which is
non-idiomatic; refactor by first iterating over apiBuckets with a forEach (or a
single reduce) to compute the tracking variables (update overallLowestRemaining,
constrainedModel, overallResetTime, hasFlashModels, flashLowestRemaining,
flashResetTime, hasProModels, proLowestRemaining, proResetTime) and then create
the quotaBuckets array with a pure map that only returns { modelId,
remainingFraction, resetTime } (or do the whole thing in one reduce that returns
both the tracking summary and the array), updating code in
gemini-usage-service.ts around the apiBuckets.map usage and referencing the same
variable names used now.
In `@apps/server/src/services/zai-usage-service.ts`:
- Around line 349-383: The code calls clearTimeout(timeoutId) twice inside
fetchUsageData: once inside the inner try after await fetch and again in the
finally block; remove the redundant inner clearTimeout call (the finally block
that clears timeoutId using the AbortController/timeoutId already guarantees
cleanup) so only the finally retains clearTimeout(timeoutId) and the
AbortController usage remains unchanged.
In `@apps/server/src/tests/cli-integration.test.ts`:
- Around line 273-278: The loop currently uses the unused destructured key name
_provider correctly, but the inner assertion
expect(result.issues.length).toBeGreaterThan(0) is tautological because the if
guard already checks result.issues.length > 0; remove that redundant assertion
and keep only the meaningful check (e.g., assert the first issue is truthy)
inside the for-each over Object.entries(results). Maintain the destructured
parameter name _provider and the variable result to locate the loop and update
its body accordingly.
In
`@apps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsx`:
- Around line 129-166: Duplicate JSX for the Logs and Stop buttons appears in
both the isCurrentAutoTask branch and the alternate branch; extract these into
two small stateless components (e.g., LogsButton and StopButton) that accept the
needed props (onViewOutput, onForceStop, shortcutKey, feature.id) and preserve
the existing handlers (e.stopPropagation in onClick and onPointerDown), testids
(`view-output-${feature.id}`, `shortcut-key-${feature.id}`,
`force-stop-${feature.id}`) and classes; then replace the four duplicated blocks
with <LogsButton .../> and <StopButton .../> calls so styling/behavior stays
centralized and future updates are applied in one place (ensure prop names match
the existing onViewOutput/onForceStop handlers).
- Line 1: The file currently uses a blanket "@ts-nocheck" which hides real
typing issues; remove that and replace it with a discriminated-union props type
that uses feature.status as the discriminant so TypeScript can narrow allowed
optional callbacks per state. Update the CardActions component props (e.g., the
Props / CardActionsProps interface and any Feature type) to declare unions like
{ feature: { status: "open"; ... }; onOpenAction?: (...) => void } | { feature:
{ status: "closed"; ... }; onCloseAction?: (...) => void } (adjust callback
names to match existing callbacks in this file) so inside CardActions you can
switch/if on feature.status and call only the callbacks valid for that branch
without type errors; also update any usages to satisfy the new prop shape.
Ensure you remove "@ts-nocheck" after types compile.
In
`@apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx`:
- Around line 115-123: Extract the duplicated execution-state logic into a
single exported helper (e.g., isFeatureInExecutionState) and replace the inline
`isInExecutionState` checks in both the Kanban card and List row with a call to
that helper; specifically, create a function (exported from a shared utils
module) that accepts the feature status string and returns true for `status ===
'in_progress' || status.startsWith('pipeline_')`, then update the
`isInExecutionState` usage in the kanban-card component (where
`isActivelyRunning` is computed) and the corresponding `list-row` computation to
call this helper so the guard logic is centralized and reused.
In
`@apps/ui/src/components/views/board-view/components/list-view/row-actions.tsx`:
- Around line 489-499: The inner guard `{handlers.onForceStop && (...)}` is
redundant because the outer branch already ensures `handlers.onForceStop` is
truthy; remove the inner conditional and directly render the Separator and
MenuItem (using StopCircle, MenuItem, DropdownMenuSeparator and
onClick={withClose(handlers.onForceStop)}) to simplify the JSX and avoid
duplicate checks.
- Around line 503-507: The conditional rendering uses a confusing
double-negation: replace the expression "!(!feature.error &&
handlers.onForceStop)" with the clearer equivalent "(feature.error ||
!handlers.onForceStop)" so the full condition becomes: !isCurrentAutoTask &&
feature.status === 'in_progress' && (feature.error || !handlers.onForceStop);
update the condition where it's used in row-actions.tsx (around the block
guarded by isCurrentAutoTask, feature.status, feature.error, and
handlers.onForceStop) to improve readability while preserving logic.
In
`@apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts`:
- Around line 10-11: The module-level `import { getElectronAPI }` is placed
after the executable statement `const logger =
createLogger('ApiKeyManagement')`; move the `getElectronAPI` import (and any
other imports) above any executable code so all imports appear at the top of the
file, e.g. place `import { getElectronAPI } from '@/lib/electron';` before the
`const logger = createLogger('ApiKeyManagement');` line and group it with the
other imports to satisfy the `import/first` lint rule.
| /** Minimal shape of a Cursor tool call used for permission checking */ | ||
| interface CursorToolCall { | ||
| shellToolCall?: { args?: { command: string } }; | ||
| readToolCall?: { args?: { path: string } }; | ||
| writeToolCall?: { args?: { path: string } }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for CursorToolCall and related field names across the codebase
rg -n "CursorToolCall|shellToolCall|readToolCall|writeToolCall" --type ts -C 1Repository: AutoMaker-Org/automaker
Length of output: 9614
🏁 Script executed:
#!/bin/bash
# Check the structure of `@automaker/types` package and its exports
find . -path "*/packages/types/*" -name "*.ts" -o -path "*/automaker/types/*" -name "*.ts" | head -20Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Look at the permission-enforcer.ts file to verify the current state
cat -n apps/server/src/lib/permission-enforcer.ts | head -50Repository: AutoMaker-Org/automaker
Length of output: 1872
🏁 Script executed:
#!/bin/bash
# Check for export statements in permission-enforcer.ts
rg -n "^export" apps/server/src/lib/permission-enforcer.tsRepository: AutoMaker-Org/automaker
Length of output: 198
Export CursorToolCall or move it to @automaker/types
The CursorToolCall interface is used as a parameter type in two exported functions (checkToolCallPermission and logPermissionViolation) but is not itself exported. This creates an ergonomic gap: callers cannot explicitly annotate variables with the type without importing it, though TypeScript's structural typing allows passing compatible objects.
More importantly, per coding guidelines ("Core TypeScript definitions with no dependencies should be isolated in the automaker/types package"), this interface should be defined in @automaker/types rather than locally. While CursorToolCallEvent already exists in the types package, the minimal shape defined here is specific to permission checking and should be extracted there and imported.
Minimal fix: Export the interface in this file, then refactor to move it to @automaker/types and import it for consistency with project conventions.
The all-optional design (allowing all three sub-call properties simultaneously) is not ideal semantically—a discriminated union would prevent invalid states—but behavior is currently correct due to priority-order checks in the implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/lib/permission-enforcer.ts` around lines 15 - 20, The
CursorToolCall interface is currently unexported but used by exported functions
checkToolCallPermission and logPermissionViolation; either export CursorToolCall
from this file now or (preferred) move its minimal definition into
`@automaker/types` and import it here (aligning with CursorToolCallEvent already
in that package). Update the local file to import the moved CursorToolCall and
adjust any references in checkToolCallPermission and logPermissionViolation to
use the shared type; keep the current all-optional shape for compatibility and
do not change the function logic.
|
|
||
| // Get effective permissions for this project | ||
| const effectivePermissions = await getEffectivePermissions(options.cwd || process.cwd()); | ||
| await getEffectivePermissions(options.cwd || process.cwd()); |
There was a problem hiding this comment.
Dead I/O — getEffectivePermissions result is discarded, making this call a no-op.
getEffectivePermissions performs up to two readFile operations and has no side effects. Calling it without consuming the return value adds latency to every executeQuery invocation with zero effect on behaviour.
The variable was removed as "unused", but that points to one of two problems:
- Permissions were never enforced — the call should be removed entirely.
- Enforcement logic was accidentally dropped — the
permissionsresult should be used to gate operations (e.g., validate that file writes/shell commands are permitted before spawning the subprocess).
🐛 Option A — remove the dead call (if permissions are not yet implemented)
- // Get effective permissions for this project
- await getEffectivePermissions(options.cwd || process.cwd());
-
// Debug: log raw events when AUTOMAKER_DEBUG_RAW_OUTPUT is enabledAlso remove the getEffectivePermissions import at line 34 if it becomes unused:
-import { getEffectivePermissions } from '../services/cursor-config-service.js';🛡️ Option B — wire up permissions enforcement (if this is the intended behaviour)
- await getEffectivePermissions(options.cwd || process.cwd());
+ const permissions = await getEffectivePermissions(options.cwd || process.cwd());
+ if (permissions) {
+ // TODO: enforce permissions before spawning subprocess
+ // e.g. reject write/shell tool calls when not allowed
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await getEffectivePermissions(options.cwd || process.cwd()); | |
| const permissions = await getEffectivePermissions(options.cwd || process.cwd()); | |
| if (permissions) { | |
| // TODO: enforce permissions before spawning subprocess | |
| // e.g. reject write/shell tool calls when not allowed | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/providers/cursor-provider.ts` at line 882, The call to
getEffectivePermissions(...) inside executeQuery is dead I/O because its return
value is discarded; either remove the call and its import (if permissions are
not used) or use the returned permissions to enforce checks before proceeding.
If removing, delete the getEffectivePermissions call and remove its import; if
enforcing, assign const permissions = await getEffectivePermissions(options.cwd
|| process.cwd()) and use permissions to validate/deny file writes or shell
command spawning in executeQuery (e.g., gate the subprocess spawn or write
operations based on permissions).
|
|
||
| // Create temporary environment override for SDK call | ||
| const cleanupEnv = createTempEnvOverride(authEnv); | ||
| const _cleanupEnv = createTempEnvOverride(authEnv); |
There was a problem hiding this comment.
_cleanupEnv is never called — process.env is permanently modified (credential leak).
createTempEnvOverride(authEnv) immediately calls Object.assign(process.env, authEnv) and returns a restore function. Since _cleanupEnv is prefixed with _ (suppressing the lint warning) but never invoked, process.env retains the injected credentials (ANTHROPIC_API_KEY, ANTHROPIC_BASE_URL, etc.) for the lifetime of the Node.js process. Every subsequent request — including requests from other users — will run under those leaked credentials/endpoints.
The finally block at Line 314 must call the cleanup function.
🔒 Proposed fix
- const _cleanupEnv = createTempEnvOverride(authEnv);
+ const cleanupEnv = createTempEnvOverride(authEnv);And in the finally block:
} finally {
clearTimeout(timeoutId);
+ cleanupEnv();
// Clean up the auth session
AuthSessionManager.destroySession(sessionId);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/routes/setup/routes/verify-claude-auth.ts` at line 154, The
temp env override created by createTempEnvOverride(authEnv) is never invoked
(stored as _cleanupEnv) so process.env retains injected credentials; ensure you
call the returned cleanup function in the verification flow's finally block:
store the result of createTempEnvOverride(authEnv) (e.g., const cleanupEnv =
createTempEnvOverride(authEnv)) and invoke cleanupEnv() inside the existing
finally block in verify-claude-auth (or wrap the try block that uses the temp
env), guaranteeing the restore runs regardless of success or error to remove
ANTHROPIC_API_KEY/ANTHROPIC_BASE_URL from process.env.
| // Check common installation paths | ||
| const possiblePaths = [ | ||
| // npm global installs | ||
| path.join(os.homedir(), '.npm-global', 'bin', 'gemini'), | ||
| '/usr/local/bin/gemini', | ||
| '/usr/bin/gemini', | ||
| // Homebrew | ||
| '/opt/homebrew/bin/gemini', | ||
| '/usr/local/opt/gemini/bin/gemini', | ||
| // nvm/fnm node installs | ||
| path.join(os.homedir(), '.nvm', 'versions', 'node'), | ||
| path.join(os.homedir(), '.fnm', 'node-versions'), | ||
| // Windows | ||
| path.join(os.homedir(), 'AppData', 'Roaming', 'npm', 'gemini.cmd'), | ||
| path.join(os.homedir(), 'AppData', 'Roaming', 'npm', 'gemini'), | ||
| ]; | ||
|
|
||
| for (const p of possiblePaths) { | ||
| if (fs.existsSync(p)) { | ||
| return p; | ||
| } | ||
| } |
There was a problem hiding this comment.
Directory paths in possiblePaths will cause findGeminiBinaryPath to return a directory instead of a binary.
Lines 459–460 include ~/.nvm/versions/node and ~/.fnm/node-versions in possiblePaths. Both are directories, and fs.existsSync() returns true for any existing path regardless of type. On any machine with nvm or fnm installed (very common among Node.js developers), the function short-circuits and returns a directory. Downstream, extractOAuthClientCredentials computes baseDir = path.dirname(resolvedDirectory) — one level above the nvm versions root — and all possibleOAuth2Paths entries are constructed from the wrong base, silently preventing OAuth credential extraction and token refresh.
🐛 Proposed fix — remove the non-binary directory entries
const possiblePaths = [
// npm global installs
path.join(os.homedir(), '.npm-global', 'bin', 'gemini'),
'/usr/local/bin/gemini',
'/usr/bin/gemini',
// Homebrew
'/opt/homebrew/bin/gemini',
'/usr/local/opt/gemini/bin/gemini',
- // nvm/fnm node installs
- path.join(os.homedir(), '.nvm', 'versions', 'node'),
- path.join(os.homedir(), '.fnm', 'node-versions'),
// Windows
path.join(os.homedir(), 'AppData', 'Roaming', 'npm', 'gemini.cmd'),
path.join(os.homedir(), 'AppData', 'Roaming', 'npm', 'gemini'),
];If nvm/fnm support is desired, enumerate specific versioned binary paths or rely solely on the which/where call at line 435 (which correctly resolves the active version).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/services/gemini-usage-service.ts` around lines 449 - 470, The
findGeminiBinaryPath implementation currently returns directory paths because
possiblePaths includes directory roots (e.g., path.join(os.homedir(), '.nvm',
'versions', 'node') and path.join(os.homedir(), '.fnm', 'node-versions')),
causing extractOAuthClientCredentials to compute the wrong baseDir and fail
OAuth discovery; remove those non-binary directory entries from possiblePaths
(or replace them with explicit versioned binary paths) so only actual gemini
executable paths are checked, or alternatively change the existence check to
confirm the path is a file/executable before returning (update
findGeminiBinaryPath and ensure extractOAuthClientCredentials still derives
baseDir from a confirmed binary path).
| getApiHost(): string { | ||
| // Priority: 1. Instance host, 2. Z_AI_API_HOST env, 3. Default | ||
| return process.env.Z_AI_API_HOST ? `https://${process.env.Z_AI_API_HOST}` : this.apiHost; | ||
| } |
There was a problem hiding this comment.
getApiHost() prepends https:// unconditionally — double-prefix if env var contains a full URL.
If Z_AI_API_HOST is set to https://api.z.ai, getApiHost() returns https://https://api.z.ai. The same guard used in setApiHost() (host.startsWith('http')) is missing here. The same issue exists in verifyApiKey() at line 246.
🐛 Proposed fix
getApiHost(): string {
- return process.env.Z_AI_API_HOST ? `https://${process.env.Z_AI_API_HOST}` : this.apiHost;
+ const envHost = process.env.Z_AI_API_HOST;
+ if (envHost) {
+ return envHost.startsWith('http') ? envHost : `https://${envHost}`;
+ }
+ return this.apiHost;
}Apply the same fix in verifyApiKey() at line 246 where Z_AI_API_HOST is also used inline.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getApiHost(): string { | |
| // Priority: 1. Instance host, 2. Z_AI_API_HOST env, 3. Default | |
| return process.env.Z_AI_API_HOST ? `https://${process.env.Z_AI_API_HOST}` : this.apiHost; | |
| } | |
| getApiHost(): string { | |
| // Priority: 1. Instance host, 2. Z_AI_API_HOST env, 3. Default | |
| const envHost = process.env.Z_AI_API_HOST; | |
| if (envHost) { | |
| return envHost.startsWith('http') ? envHost : `https://${envHost}`; | |
| } | |
| return this.apiHost; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/services/zai-usage-service.ts` around lines 172 - 175,
getApiHost() unconditionally prefixes Z_AI_API_HOST with "https://" which yields
double-protocol strings if the env var already contains a scheme; change the
logic so when process.env.Z_AI_API_HOST is present you first check if it
startsWith('http') and return it as-is, otherwise prepend 'https://'; apply the
identical pattern to the inline usage of process.env.Z_AI_API_HOST in
verifyApiKey() so both functions (getApiHost and verifyApiKey) consistently
handle full URLs and bare hostnames.
| async configure( | ||
| options: { apiToken?: string; apiHost?: string }, | ||
| settingsService: SettingsService | ||
| ): Promise<ConfigureResult> { | ||
| const emitter = createEventEmitter(); | ||
|
|
||
| if (options.apiToken !== undefined) { | ||
| // Set in-memory token | ||
| this.setApiToken(options.apiToken || ''); | ||
|
|
||
| // Persist to credentials | ||
| try { | ||
| await settingsService.updateCredentials({ | ||
| apiKeys: { zai: options.apiToken || '' }, | ||
| } as Parameters<typeof settingsService.updateCredentials>[0]); | ||
| logger.info('[configure] Saved z.ai API key to credentials'); | ||
| } catch (persistError) { | ||
| logger.error('[configure] Failed to persist z.ai API key:', persistError); | ||
| } | ||
| } | ||
|
|
||
| if (options.apiHost) { | ||
| this.setApiHost(options.apiHost); | ||
| } | ||
|
|
||
| const result: ConfigureResult = { | ||
| success: true, | ||
| message: 'z.ai configuration updated', | ||
| isAvailable: this.isAvailable(), | ||
| }; | ||
|
|
||
| emitter.emit('notification:created', { | ||
| type: 'zai.configured', | ||
| success: result.success, | ||
| isAvailable: result.isAvailable, | ||
| }); | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| /** | ||
| * Verify an API key without storing it. |
There was a problem hiding this comment.
Local createEventEmitter() instances have no subscribers — all emit calls are no-ops.
Each method (configure, verifyApiKey, fetchUsageData) creates a brand-new EventEmitter via createEventEmitter(). As seen in lib/events.ts, a freshly created emitter holds an empty Set<EventCallback>. Since nothing subscribes to these local instances before the methods return, every emitter.emit(...) call silently discards the event.
The correct pattern — used by services like RecoveryService — is to inject a shared event bus (e.g., TypedEventBus) via the constructor and call emit on that shared instance:
// Constructor injection (mirrors RecoveryService pattern)
export class ZaiUsageService {
constructor(private eventBus: TypedEventBus) {}
async configure(...) {
// ...
this.eventBus.emit('notification:created', { type: 'zai.configured', ... });
}
}Without this, the UI can never receive z.ai lifecycle events over WebSocket.
Based on learnings: "All server operations should emit events using createEventEmitter() from lib/events.ts that stream to the frontend via WebSocket."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/services/zai-usage-service.ts` around lines 189 - 230, The
methods configure, verifyApiKey and fetchUsageData currently call
createEventEmitter() locally which produces a new emitter with no subscribers,
so emitter.emit(...) are no-ops; modify the ZaiUsageService to accept a shared
TypedEventBus (or the shared emitter) via constructor injection (e.g.,
constructor(private eventBus: TypedEventBus) {}) and replace local
createEventEmitter() usages with this.eventBus.emit(...) (referencing configure,
verifyApiKey, fetchUsageData and any other methods using createEventEmitter());
ensure the constructor parameter is used throughout and remove the local
createEventEmitter() calls so emitted 'notification:created' events are
delivered to subscribers.
| const fetchZaiUsage = useCallback(async () => { | ||
| setIsZaiLoading(true); | ||
| try { | ||
| const api = getElectronAPI(); | ||
| if (!api.zai) return; | ||
| const data = await api.zai.getUsage(); | ||
| if (!('error' in data)) { | ||
| setZaiUsage(data); | ||
| } | ||
| } catch { | ||
| // Silently fail - usage display is optional | ||
| } finally { | ||
| setIsZaiLoading(false); | ||
| } | ||
| }, [setZaiUsage]); |
There was a problem hiding this comment.
fetchZaiUsage doesn't pass a timestamp to setZaiUsage — zaiUsageLastUpdated may never be set.
fetchGeminiUsage (line 260) correctly calls setGeminiUsage(data, Date.now()), updating geminiUsageLastUpdated and enabling the 2-minute stale window. fetchZaiUsage (line 244) calls setZaiUsage(data) with no timestamp.
If the setZaiUsage store implementation doesn't implicitly update zaiUsageLastUpdated, the stale check at line 201 will always be true and z.ai usage will be fetched on every component mount with no cache benefit. This also contradicts the symmetric treatment of all providers in the stale-check logic (lines 198–203).
Either pass Date.now() explicitly (requires updating the setZaiUsage type signature to match setGeminiUsage) or ensure the store setter implicitly sets zaiUsageLastUpdated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/board-view/mobile-usage-bar.tsx` around lines
237 - 251, fetchZaiUsage currently calls setZaiUsage(data) without a timestamp
so zaiUsageLastUpdated may never be set; mirror fetchGeminiUsage's behavior by
passing a timestamp to the store setter (e.g., call setZaiUsage(data,
Date.now())), and update the setZaiUsage type/signature where it’s defined to
accept the optional/required timestamp (similar to setGeminiUsage) so the
stale-check using zaiUsageLastUpdated works and z.ai usage is cached for the
2-minute window.
| try { | ||
| const api = getElectronAPI(); | ||
| // Use the verify endpoint to test the key without storing it | ||
| const response: VerifyResponse | undefined = await api.zai?.verify(zaiKey); | ||
|
|
||
| if (response?.success && response?.authenticated) { | ||
| setZaiTestResult({ | ||
| success: true, | ||
| message: response.message || 'Connection successful! z.ai API responded.', | ||
| }); | ||
| } else { | ||
| setZaiTestResult({ | ||
| success: false, | ||
| message: response?.error || 'Failed to connect to z.ai API.', | ||
| }); | ||
| } | ||
| } catch { | ||
| setZaiTestResult({ | ||
| success: false, | ||
| message: 'Network error. Please check your connection.', | ||
| }); | ||
| } finally { | ||
| setTestingZaiConnection(false); | ||
| } |
There was a problem hiding this comment.
getElectronAPI() can return null — api.zai?.verify will throw TypeError when it does.
Line 229 does not guard against a null return from getElectronAPI(). The optional chain api.zai?.verify(...) only protects against api.zai being absent, not against api itself being null. This is inconsistent with the pattern used at line 98 (if (api?.setup?.getApiKeys)), and the resulting TypeError would be silently swallowed as a misleading "Network error. Please check your connection." message.
🛡️ Proposed fix
try {
const api = getElectronAPI();
+ if (!api?.zai?.verify) {
+ setZaiTestResult({ success: false, message: 'z.ai verification is unavailable in this environment.' });
+ setTestingZaiConnection(false);
+ return;
+ }
// Use the verify endpoint to test the key without storing it
- const response: VerifyResponse | undefined = await api.zai?.verify(zaiKey);
+ const response: VerifyResponse | undefined = await api.zai.verify(zaiKey);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const api = getElectronAPI(); | |
| // Use the verify endpoint to test the key without storing it | |
| const response: VerifyResponse | undefined = await api.zai?.verify(zaiKey); | |
| if (response?.success && response?.authenticated) { | |
| setZaiTestResult({ | |
| success: true, | |
| message: response.message || 'Connection successful! z.ai API responded.', | |
| }); | |
| } else { | |
| setZaiTestResult({ | |
| success: false, | |
| message: response?.error || 'Failed to connect to z.ai API.', | |
| }); | |
| } | |
| } catch { | |
| setZaiTestResult({ | |
| success: false, | |
| message: 'Network error. Please check your connection.', | |
| }); | |
| } finally { | |
| setTestingZaiConnection(false); | |
| } | |
| try { | |
| const api = getElectronAPI(); | |
| if (!api?.zai?.verify) { | |
| setZaiTestResult({ success: false, message: 'z.ai verification is unavailable in this environment.' }); | |
| setTestingZaiConnection(false); | |
| return; | |
| } | |
| // Use the verify endpoint to test the key without storing it | |
| const response: VerifyResponse | undefined = await api.zai.verify(zaiKey); | |
| if (response?.success && response?.authenticated) { | |
| setZaiTestResult({ | |
| success: true, | |
| message: response.message || 'Connection successful! z.ai API responded.', | |
| }); | |
| } else { | |
| setZaiTestResult({ | |
| success: false, | |
| message: response?.error || 'Failed to connect to z.ai API.', | |
| }); | |
| } | |
| } catch { | |
| setZaiTestResult({ | |
| success: false, | |
| message: 'Network error. Please check your connection.', | |
| }); | |
| } finally { | |
| setTestingZaiConnection(false); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts`
around lines 228 - 251, The code calls getElectronAPI() without guarding for a
null return, so api.zai?.verify can throw; update the verify flow in the
function that contains getElectronAPI() (the block using api.zai?.verify and
calling setZaiTestResult/setTestingZaiConnection) to first check api is present
(e.g., if (!api || !api.zai || typeof api.zai.verify !== 'function') ), and if
missing setZaiTestResult to a clear error about the desktop API not being
available and skip calling verify; keep the existing catch/finally behavior but
ensure you return early when the API is absent to avoid the TypeError.
Add usage tracking for Gemini and Z.ai coding plan models. Improve task recovery and UI state sync. Remove unused variables.
Summary by CodeRabbit